RHOAIENG-10783: fix(rocm): remove more files that instructlab also removes #653
Conversation
jiridanek
requested review from
harshad16
and removed request for
caponetto and
paulovmr
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Looks good, but we should really assure that this isn't missing in the final image for any use-case we should support. |
| @@ -18,6 +18,9 @@ WORKDIR /opt/app-root/bin | |||
| ARG ROCM_VERSION=6.1 | |||
| ARG AMDGPU_VERSION=6.1 | |||
|
|
|||
| # default: same targets and ROCm version as https://github.com/tiran/instructlab-containers/blob/main/containers/rocm/Containerfile.c9s#L47 | |||
| ARG AMDGPU_TARGETS=gfx900;gfx906:xnack-;gfx908:xnack-;gfx90a:xnack-;gfx90a:xnack+;gfx942;gfx1030;gfx1100 | |||
There was a problem hiding this comment.
Can we get some official set of AMD GPUs we should/need to support? I mean - are we sure that this default is what we want?
There was a problem hiding this comment.
Looks good, but we should really assure that this isn't missing in the final image for any use-case we should support.
It's much better to miss something and add it later, than trying to remove things afterwards when we actually have users, imo.
There was a problem hiding this comment.
Plan is to support: AMD MI210, MI300 and later on MI250
Based on the strat we have https://issues.redhat.com/browse/RHOAISTRAT-135.
i agree, we can add the details if the opportunity presents, and try this out for now.
however what are these gfx files, are we aware what functionality is this removing ?
jiridanek
changed the title
…oves Turns out we can remove the llvm installation if we do it forcibly, without removing dependencies. Additionally, there are gfx files for all supported cards, so since we support less, we can remove many. See https://github.com/tiran/instructlab-containers/blob/main/containers/rocm/Containerfile.c9s#L47
|
/test ci/prow/images |
|
@jiridanek: The specified target(s) for
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test images |
|
@harshad16 The
|
|
/retest all |
|
@jiridanek: The
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test all |
|
@jiridanek: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@jiridanek , with this #673 getting merged |
|
not planned for now |
Description
Turns out we can remove the llvm installation if we do it forcibly, without removing dependencies. Additionally, there are gfx files for all supported cards, so since we support less, we can remove many.
See https://github.com/tiran/instructlab-containers/blob/main/containers/rocm/Containerfile.c9s#L47
How Has This Been Tested?
before
after
Merge criteria: